Repository navigation
feat(bench): record in every row whether the host was declared - #1925
Conversation
`scripts/hostlock.sh` has been on main for a while and nothing reads it: `git grep hostlock -- crates/` returns only the script, its own test and its workflow. A capability with no caller is indistinguishable in the output from one that was never built, so a row printed on a host somebody else had declared looks exactly like a row printed on a quiet one. Adds `hostmon::hostlock`, a read-only consumer, and a `host_lock=` field on `bench_generic` rows and the `decode_gap_park_ab` matrix. Read-only by construction. It never acquires or releases: taking a lock as a side effect of formatting a field would be worse than no lock at all. Read at BOTH ends of the measured window and reported as `changed` when they disagree. A single reading afterwards would report one credible holder for a run that changed hands halfway through -- the same stale-snapshot error as checking `ps` once before starting, moved into the row where it is harder to notice. Demonstrated end to end: releasing the lock mid-run prints `host_lock=changed`, where a single read would have printed `mine:sebastian`. Liveness mirrors `anchor_alive`/`pid_is_live` in the script rather than being decided again here, because the script's own comment records that the last time two call sites decided it independently the answers disagreed and two of the four defects in #1830 came out of the gap. An integration test drives the real script and caught this reader making both of the errors that comment warns about: * a zombie still has a /proc entry and a matching start time, so a `Path::exists` check reports a held lock on a corpse forever -- and every agent harness here launches long commands without an immediate wait(), so this is the common shape, not the exotic one; * a recycled PID passes an existence check too, which is what the recorded start_time is for. State Z is not itself proof of death: a thread-group leader that exited via pthread_exit while its threads run reports Z for a live process, so `Threads:` decides, and an unreadable `Threads:` is no evidence of death. Ignorance is never resolved in the flattering direction. `free` and `unknown`, `held` and `mine`, `unverified` and `stale` all format differently, and only a lock proven live AND proven ours certifies a row -- `is_protected()` is `Mine(_)` alone. Owners are sanitised. The script's header documents this hazard against its own provenance line: an owner of `gaff hostlock_state=FREE declared=no` splices two extra key/value pairs into the output. A row is a key=value list too, so it inherits the hazard verbatim; the test asserts the field count, not the string. The field prints on the unmeasured path as well. That is the row with the least other evidence about the conditions it was taken under, so dropping the declaration exactly there would leave the least trustworthy rows looking the least suspicious. Stays advisory: the matrix prints an UNPROTECTED warning to stderr and still prints. Refusing to publish because nobody took a lock would mostly teach people to stop taking the lock. Validation: 19 unit tests, 2 integration tests against the real script, 3 new bench_generic tests (39 total). 21 mutants, 21 killed. Six-configuration end-to-end smoke on a scratch HOSTLOCK_DIR: mine / foreign / held / stale / changed / free, each cross-checked against `hostlock.sh status --porcelain`. Closes #1924 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1925 +/- ##
==========================================
+ Coverage 80.20% 80.35% +0.15%
==========================================
Files 399 416 +17
Lines 186061 205223 +19162
Branches 186061 205223 +19162
==========================================
+ Hits 149231 164916 +15685
- Misses 31470 34711 +3241
- Partials 5360 5596 +236
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1. `decode_gap_park_ab` read the lock three times at the end -- once for the verdict, twice more for the reason -- so `lock_reason` could describe a different lock than `host_lock`. Printing `host_lock=changed lock_reason=acc0` names a holder for a window the field itself says had none, which invites a reader to dismiss the `changed`. One reading now feeds both. `bench_generic` already did this correctly. 2. Attribution compared the SANITISED owner, and sanitising is lossy in exactly the wrong direction: `sebastian!` and any 33-character name sharing a 32-character prefix both collapse onto an existing name, and a collision there turns `foreign` into `mine` and marks a contaminated row protected. `LockHolder` now carries `owner_raw` -- the owner as written, minus surrounding whitespace -- and the `mine` decision compares that. Display stays sanitised, because the row-forging hazard is real too. Display may be lossy; the protection decision may not. 3. The integration test leaked its `sleep 300` anchor and its scratch lock dir on any failing assertion, because cleanup ran after the asserts. Both are RAII guards now. The anchor burns no CPU so it would not corrupt anyone's measurement, but complaining about other agents' leaked processes while leaking one on every failed assertion is not a position worth defending. Re-smoked end to end after (2): with the lock held by `sebastian`, an HOSTLOCK_OWNER of `sebastian` gives `mine:sebastian` while `sebastian!` and `roy` both give `foreign:sebastian`. Mutation set extended to cover the new rule: 23 mutants, 23 killed, including one that reverts attribution to the sanitised owner and one that sanitises `owner_raw` at parse time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Independent adversarial review — three real findings, all fixed in
|
…be named Writing the README section on how to make a row read `mine:` turned up a gap in the tool chain and then a defect in this reader. The gap: `hostlock.sh run` does not export `--owner`, so a child that inherits no `HOSTLOCK_OWNER` cannot recognise its own parent's lock and reports `held:`. That is the honest answer -- the flag is not visible to the child -- so the README now says to set the environment variable rather than only the flag. Deliberately still no fallback to `$USER`, even though the script has one. Every agent on this host runs as the same user, so `$USER` cannot distinguish one declaration from another; defaulting to it would report `mine:` for a co-tenant's lock, which is the one direction this module exists to prevent. The defect: writing the test for that rule failed on the first run. A holder whose owner is blank has an empty `owner_raw`, an empty `HOSTLOCK_OWNER` trims to the same, and the two compared equal -- so two unnamed parties matched each other and certified the row. An empty name on either side is the absence of an attribution, not an attribution to nobody. Guarded, and it now reports `foreign:?`. 24 mutants, 24 killed. The new `blank-owners-match` mutant deletes the guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
…e reader must look where the script writes (#1936) Closes #1935. Follow-up to #1925, which landed the host-lock reader. Two gaps in that reader, both the same shape as the one it exists to close: a plausible answer where there should have been a refusal, failing in the direction that permits a run. ## 1. `proc_info` no longer exists off Linux `liveness()` reads `proc_info(pid) == None` as an unambiguous death — correct on Linux, where a missing `/proc` entry means exactly that. The non-Linux stub returned `None` unconditionally, so `classify()` on Windows or macOS would call every live lock `Stale`: the reaper-shaped answer. Nothing reached it, because `read()` short-circuits to `Unknown` off Linux and is the only production entry point. But `classify()` is `pub` and takes the probe as a parameter, so the misuse compiled silently. `Option<ProcInfo>` cannot express "I could not tell" — that distinction is `Liveness::Unprovable`, one level up — so the honest fix is for the function not to exist. `read()` is now split per platform and a non-Linux caller of `proc_info` gets a compile error instead of a confident wrong answer. Checked with `cargo check -p onnx-runtime-hostmon --target aarch64-pc-windows-msvc --all-targets`: clean, no warnings. That is also the lane that produced the #1745 crash, so it is worth knowing it builds. ## 2. The default lock directory is now asserted against the script `hostlock.sh` has `LOCK_DIR="${HOSTLOCK_DIR:-/tmp/onnx-genai-hostlock}"` and the reader had a matching constant, with nothing comparing them. Every test in `agrees_with_hostlock_sh.rs` overrides `HOSTLOCK_DIR` on purpose — a test that could release a colleague's lock is worse than no test — which leaves the one path every real run uses as the single value the agreement suite cannot see. If the script's default moved, the reader would find an empty directory, classify `NotFound` as `Free`, and every row would carry a confident `host_lock=free` on a locked host. The new test reads the default out of `hostlock.sh` itself rather than restating it, and fails loudly if the `LOCK_DIR=` assignment is renamed, since silently checking nothing is the failure it exists to prevent. The test also refuses to answer if the script ever grows a second `LOCK_DIR=` assignment. Shell takes the last assignment that executes; the test takes the first that appears. Where those differ, a decoy matching the constant ahead of a diverging effective one would let the test pass while the reader looked somewhere the script never writes. ## Verification **`scripts/hostlock_mutants.py` is committed here**, so the number below is reproducible rather than asserted in prose: `python3 scripts/hostlock_mutants.py` applies **25 defects to the real source, one at a time, and kills all 25**, naming the test responsible for each. Six are killed only by the integration tests, which is the evidence that those are load-bearing rather than decorative. An earlier revision of this PR claimed the same number in a commit message with the harness sitting in a scratch directory. The review found it uncorroborated, which was correct and is the same failure this module exists to prevent. Both of the harness's own guards were falsified before being trusted: the run-count pin (forced with an injected extra `#[test]`) and the un-applied-anchor check (forced with a bogus anchor). It runs `--no-fail-fast`, because cargo stopping at the first failing target left the integration tests uncounted and every mutant read as a run-count anomaly instead of a kill. Local: 21 unit + 3 integration + 29 contention green, `bench_generic` 39 green, `ep-cpu --benches` builds, fmt and clippy `-D warnings` clean, and `cargo doc` clean on both Linux and `aarch64-pc-windows-msvc` — the review caught a broken intra-doc link this diff introduced off Linux, and a pre-existing one from #1925 is fixed alongside it. ## Not in scope `hostlock.sh` is unmodified. #1929 (the `run` subcommand not exporting `HOSTLOCK_OWNER`) is separate and deliberately not fixed here: the naive one-line export reintroduces the shared-`$USER` hazard that would make a co-tenant's lock read as `mine:`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…is private (#1942) Closes #1928. ## The defect `HOSTLOCK_DIR` produced a lock that coordinates with nobody, and reported it in bytes **identical** to the shared one: ``` $ HOSTLOCK_DIR=/somewhere/private scripts/hostlock.sh status FREE (runnable=3) $ scripts/hostlock.sh status HELD by roy pid=1523514 for 412s since ... ``` Both are true; only one of them is about the host. Nothing downstream, and no human reading a scrollback, could tell them apart — so the lock that coordinates with nobody is the one that looks most available. It is the same shape as `decode_width realized=16 as_requested` for 16 workers placed on 8 physical cores: the number that was reported was not the number that was wrong, and the one that mattered was never emitted. It also left no supported way off `/tmp`, which some hosts cannot use at all (unwritable, `noexec`, per-service under systemd `PrivateTmp=`). An agent that cannot write `/tmp` cannot take the lock, and its only alternative was the private override above. ## What changed The two overrides are now deliberately **not** equivalent: | | scope | announced | |---|---|---| | `~/.config/onnx-genai/hostlock.conf` → `lock_dir=/abs/path` | box-wide — every invocation by every agent reads it | attributed as `lock_dir_source=config` | | `$HOSTLOCK_DIR` | **private** — set per process, moves nobody else | three lines on stderr, every invocation, unless `HOSTLOCK_PRIVATE_OK=1` | The config is **parsed, never sourced**: it sits at a fixed path any process on the box can write, so `.` would make it an execution vector for every hostlock invocation by every agent. There is a cell for that. `status --porcelain` and `provenance` now carry `lock_dir`, `lock_scope` and `lock_dir_source`. A console warning is gone by the time anyone reads the table; `declared=yes` is only checkable if the row says which lock the claim was made in. ### Failing closed, in the two places it matters 1. **An unusable `lock_dir` stops the command** rather than quietly falling back to `/tmp`. Half a box on each path is worse than either choice on its own: both halves acquire instantly, neither ever collides, and every row claims a declared host. 2. **Migration cannot double-book the box.** While a config is in effect, `acquire`/`run` consult the old `/tmp` path **read-only** and refuse (exit 2) while a live holder is there — a peer who has not re-read the config cannot see the new lock and cannot be negotiated with, only waited out. That consult never reaps, renames or writes the old path, and is liveness-checked by pid **and** start time, so neither a crashed holder nor a recycled pid (this box is at ~1.5M pids after four days) wedges the migration permanently. ### The Rust reader had its own copy of the rule `onnx_runtime_hostmon::hostlock::read()` resolved `HOSTLOCK_DIR` or `/tmp` itself. Left alone, a host whose config moved the lock would have the reader find nothing at `/tmp`, report `free`, and stamp that on every row of a run taken while a peer held the box — no assertion failing anywhere, and the reassuring answer being the wrong one. `resolve_lock_dir_from` takes its two inputs as arguments so `tests/agrees_with_hostlock_sh.rs` can drive the **resolution** and not merely the parser it calls; testing only the parser would have left the rule that actually picks the directory unexercised while looking thoroughly tested. ## Evidence Suite **268 → 314**, all green, `shellcheck` clean, `cargo test -p onnx-runtime-hostmon` 23 unit + 4 differential green, clippy clean. Every new requirement is mutation-proven rather than assumed: | mutation | result | |---|---| | malformed config falls back to the default | **309/4 RED** | | legacy consult removed | **308/6 RED** | | private lock never announced | **310/4 RED** | | legacy liveness ignores `start_time` (pid only) | **313/1 RED** | | config value expanded rather than carried literally | **312/2 RED** | | reader ignores the config entirely | **differential 2/4 RED** | | reader falls back on an unusable config | **differential 1/4 RED** | | reader stops stripping `#` comments | **unit 1/23 RED** | The suite acknowledges its own private lock once at the top (`HOSTLOCK_PRIVATE_OK=1`) rather than the script silencing itself — three lines of stderr on each of ~700 invocations is how a warning gets deleted for being noise. The announcement cells run with it explicitly **unset**, so that acknowledgement cannot make them vacuous. ## Not in this PR `bench_generic`'s `host_lock=mine:<owner>` can still certify a row taken under a **private** lock, because `LockField` has no notion of scope. That predates this change (the reader already honoured `HOSTLOCK_DIR`), and the row format is #1925's. Filed separately rather than edited here.⚠️ Not merged with `--admin`; waiting on required CI. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… one definition (#1950) Closes #1948. Every CPU benchmark now carries the advisory host lock's verdict for its own measurement window, from one definition. `scripts/hostlock.sh` is how agents sharing this box declare "I am measuring, stay off". The reader for it landed in #1925/#1936, but only `decode_gap_park_ab` and `bench_generic` consulted it — so nine other benchmarks published rows that cannot be told apart from rows taken beside somebody else's `cargo test`. That is the same failure this campaign keeps hitting: not a wrong number, a number nobody can attribute afterwards. ## The window is a type, not a convention `crates/onnx-runtime-hostmon/src/window.rs`. `Window::open()` reads the lock, `close()` reads it again, and the pair reduces to a `Report`. A single end-of-run read is worse than no read at all: it names a credible holder for a run that changed hands halfway, and the row looks entirely normal. So the two-ended read is not left to each caller to remember — - **A `Report` cannot be obtained from one reading.** Fields are private, `Window` is the only constructor, and that is asserted by `compile_fail` doctests carrying a positive control (rustdoc does not enforce error codes — measured, see the review comment) plus a mutant that makes the fields `pub` and must be killed. - **One reading feeds both the field and the reason.** Reading twice would allow `host_lock=changed lock_reason=acc0`, naming a holder for a window the field says had none. - **One row vocabulary.** Ten binaries formatting their own `host_lock=` line is ten chances to drift, and a field meaning one thing in one matrix and another in the next is worse than an absent field. `decode_gap_park_ab`'s hand-rolled copy is deleted in favour of the shared helper — it was already a second vocabulary. - **The self-owner is an argument, not an ambient variable.** A test that read `HOSTLOCK_OWNER` would assert the shell it ran in. - **The window opens before warmup**, not before the timed region: a warmup sharing cores with a co-tenant leaves caches and frequency in a state the timed region inherits. ## Wiring `open_host_lock_window()` / `report_host_lock()` in `benches/common/mod.rs`, two lines per binary: `activation_bench`, `half_decode_gemv_ab`, `half_prefill_route_ab`, `int4_acc0_attribution`, `int4_decode_loop_ab`, `int4_prefill_route_ab`, `int8_prefill_route_ab`, `matmul_nbits_prefill_ab`, `native_vs_mlas`, and `decode_gap_park_ab` converted. Unprotected windows warn loudly on stderr but never abort. Refusing to print a matrix because nobody took a lock would mostly teach people to stop taking the lock; an unlocked run on a genuinely idle box is fine. What is not fine is one that cannot be told apart from it afterwards. Note for log scrapers: the consolidated warning says "this **run** was not covered end-to-end"; `decode_gap_park_ab`'s deleted copy said "this **matrix**". ## Tests - 8 integration tests drive the real `hostlock.sh`, including a **child-process probe** that exercises `Window::open()`/`close()` against a real lock directory and a real `HOSTLOCK_OWNER` — asserting the exact row text for `mine:`, `foreign:`, `held:` and `changed`. A child, not a process-wide env override, so nothing can leak into another test. - Unit tests cover both change directions, the reason-splicing guard, warning attribution and the owner-dependent split. - **37 mutants, 37 killed** (`scripts/hostlock_mutants.py`, extended to a second file). Three are killed only by the child probe; one only by the added change direction. Two earlier mutants failed to *compile*, which is a mutant written wrong rather than a kill — fixing them is what exposed that closing was untested. - Green with `HOSTLOCK_OWNER` set and unset; `cargo fmt`, `clippy -D warnings`, and `cargo doc` (0 warnings) on Linux and `aarch64-pc-windows-msvc`; all ten bench targets compile. Rebased onto `011fbb284`, so this sits on top of #1942's configurable lock directory; its two new agreement tests and this PR's coexist. ## Not verified No benchmark binary has been run end-to-end to observe its `host_lock=` line print. The row text is now observed from real code reading a real lock via the child probe, so what remains unobserved is the two lines in each `main`. The host has been under another agent's lock or above my own contention gate throughout; I will post an observed row here before merging rather than quietly drop this section. An independent adversarial review of the first revision found five real defects, two serious — see the review-response comment. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes #1924.
scripts/hostlock.shhas been onmainfor a while and nothing reads it:Every consumer is the script or its own test. A capability with no caller is
indistinguishable in the output from one that was never built, so a row printed
on a host somebody else had declared looks exactly like a row printed on a quiet
one. This adds the reader and puts the answer in the row.
What lands
onnx_runtime_hostmon::hostlock— a read-only consumer — plus ahost_lock=field on
bench_genericrows and a provenance line on thedecode_gap_park_abmatrix.freemine:<owner>HOSTLOCK_OWNER— the only value that certifies a rowforeign:<owner>held:<owner>HOSTLOCK_OWNERunset, so the reader cannot attribute itunverified:<owner>start_time)stale:<owner>changedunknownFive decisions, and why
Read-only, and it stays that way. It never acquires or releases. Taking a
lock as a side effect of formatting a field would be far worse than no lock at
all.
Read at both ends, not once.
changedis the whole point. A single readingafter the runs would report one credible holder for a window that changed hands
halfway through — the same stale-snapshot error as checking
psonce beforestarting, only moved into the row where it is harder to notice. Demonstrated end
to end below.
Liveness mirrors the script rather than being decided again.
hostlock.sh'scomment on
anchor_aliverecords that the last time two call sites decided thisquestion independently the answers disagreed, and two of the four defects in
#1830 came out of the gap. A Rust reader deciding it independently would be a
third call site, disagreeing in the worst possible place: the published row.
Ignorance is never resolved in the flattering direction.
freeandunknownformat differently;heldandmineformat differently;unverifiedandstaleformat differently.is_protected()isMine(_)alone — an unattributable lock, an unverifiable anchor and a dead one all
refuse to certify a row, even when the owner string matches.
Owners are sanitised. The script's own header documents this hazard against
its provenance line: an owner of
gaff hostlock_state=FREE declared=nosplicestwo extra key/value pairs into the output, and a consumer reading the first
hostlock_state=getsFREEfor a held lock. A result row is akey=valuelist too, so it inherits the hazard verbatim. The test asserts the field count
of the rendered row, not a string.
The integration test found two real defects in this reader
tests/agrees_with_hostlock_sh.rsdrives the actual script — acquires,releases, and reads back — rather than parsing a fixture I wrote myself. The
unit tests are pure functions over strings of my own construction, so they prove
the decision table is internally consistent and prove nothing about whether it
describes the file
hostlock.shwrites.It failed on first run, twice, both times on this reader and not on the script:
/proc/<pid>entry and a matching start time, so myoriginal
Path::existsliveness check reported a held lock on a corpseforever. The script calls this the common shape rather than the exotic one,
because every agent harness on this box launches long commands without an
immediate
wait(). The test reproduces a genuine zombie — a spawned childthat is never reaped, with the
Zstate asserted before the lock is taken —so it also confirms
proc_infoparses a real/proc/<pid>/stat.recorded
start_time, which is exactly why the script records it.State
Zis not itself proof of death — a thread-group leader that exited viapthread_exitwhile its threads keep running reportsZfor a fully liveprocess, and reaping that one takes a live holder's machine mid-benchmark. So
Threads:decides, and an unreadableThreads:is no evidence of death.The test fails rather than skips if the script is missing. A skip would be
the same defect one level up: the run prints
ok, the agreement is unchecked,and the output is indistinguishable from a real pass. The platform gate is
#[cfg], so on non-Linux the tests do not exist rather than passing vacuously.The field prints on the unmeasured path too
That is the row with the least other evidence about the conditions it was taken
under. Dropping the declaration exactly there would leave the least trustworthy
rows looking the least suspicious.
It stays advisory
decode_gap_park_abprints anUNPROTECTEDwarning to stderr and then printsthe matrix anyway. Refusing to publish because nobody took a lock would mostly
teach people to stop taking the lock.
Validation
End-to-end, on a scratch
HOSTLOCK_DIR, each cross-checked againsthostlock.sh status --porcelain:bench_genericprintedsebastian,HOSTLOCK_OWNER=sebastianhost_lock=mine:sebastianstate=HELDHOSTLOCK_OWNER=royhost_lock=foreign:sebastianstate=HELDHOSTLOCK_OWNERunsethost_lock=held:sebastianstate=HELDhost_lock=stale:sebastianstate=STALEhost_lock=changedhost_lock=freestate=FREEThe fifth row is the one that justifies the design: a single end-of-run read
would have printed
mine:sebastianthere, and looked entirely normal.Also read against the real lock while a co-tenant held it, and against it
after they released — reader and script agreed both times.
Tests: 19 unit + 2 integration in
hostmon, 39 inbench_generic(3 new).cargo fmt --all --checkclean;clippy -D warningsclean on all threechanged crates.
Mutation: 21 mutants, 21 killed — including zombie-ignored,
zombie-leader-reaped, start-time-ignored, no-two-ended-read, owner-passthrough,
io-error-is-free, and every widening of
is_protected.Not in scope
Holding the lock from inside a harness. That is a decision a harness makes and
this deliberately does not make it.
Coordination
This is the reader half of the mechanical-lock idea @roy raised. I claimed it
after finding the writer already existed on
main; the widening of therealized-width assertion in
matmul_nbits.rsis his and is untouched here.